Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mark YUV encodings as deprecated #247

Merged
merged 3 commits into from
Sep 4, 2024

Conversation

christianrauch
Copy link
Contributor

#214 introduced new encoding names uyvy and yuyv as replacement for yuv422 and yuv422_yuy2. This PR marks those replaced encodings as deprecated with a hint which encoding to use instead and removes their usage in this repo.

Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
@christianrauch christianrauch force-pushed the yuv_deprecation branch 2 times, most recently from cad674b to da6770d Compare May 11, 2024 16:58
Copy link
Contributor

@tfoote tfoote left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Marking these as deprecated I think makes sense, but we need to have a transition time before they are removed from the implementation as removing them from the implementation will break any current users. The cost of keeping these is moderately low so we should consider a moderately long deprecation cycle. More than just the minimum one release cycle.

sensor_msgs/include/sensor_msgs/image_encodings.hpp Outdated Show resolved Hide resolved
Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
@christianrauch
Copy link
Contributor Author

The CI marks this as "unstable", probably because there are 6 new warnings (https://build.ros2.org/job/Rpr__common_interfaces__ubuntu_noble_amd64/24/gcc/new/) from the "deprecated-declarations" that are introduced by this PR. @tfoote I guess keeping on using the deprecated encodings is not an option anymore?

@tfoote
Copy link
Contributor

tfoote commented May 14, 2024

You need to explicitly suppress those warnings that are known to the developer for backwards compatibility.

https://stackoverflow.com/questions/13459602/how-can-i-get-rid-of-deprecated-warnings-in-deprecated-functions-in-gcc

Luckily clang will pick up the GCC ones

https://clang.llvm.org/docs/UsersManual.html#controlling-diagnostics-via-pragmas

Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
@christianrauch
Copy link
Contributor Author

I wrapped the code that is still using the deprecated marked encodings inside the GCC diagnostic pragma:

#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
[...]
#pragma GCC diagnostic pop

@christianrauch
Copy link
Contributor Author

@tfoote Can you have a look at this again?

Copy link
Contributor

@tfoote tfoote left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking the time to mark these as deprecated. This will be helpful going forward. The change looks good. Sorry for the high latency, I'm on leave at the moment. I will come back to this for merging when I'm back in the office to avoid disruptions when I can't pay close attention unless someone else gets to it sooner.

@christianrauch
Copy link
Contributor Author

@tfoote Is now a good time to have a look at this? :-)

@tfoote tfoote merged commit 1d1c294 into ros2:rolling Sep 4, 2024
3 checks passed
@christianrauch christianrauch deleted the yuv_deprecation branch September 5, 2024 04:28
@clalancette
Copy link
Contributor

Please make sure to run CI on these PRs; this one has caused regressions.

@clalancette
Copy link
Contributor

Besides causing warnings on all platforms, the pragmas used here for GCC do not work on Windows. I'm going to revert this commit, because it causes too many problems right now.

clalancette added a commit that referenced this pull request Sep 5, 2024
@christianrauch
Copy link
Contributor Author

Please make sure to run CI on these PRs; this one has caused regressions.

The CI on this PR passed (https://build.ros2.org/job/Rpr__common_interfaces__ubuntu_noble_amd64/27/).

Of course, this CI does not test other repos and as far as I understood, causing warnings such as warning: ‘sensor_msgs::image_encodings::YUV422’ is deprecated: use sensor_msgs::image_encodings::UYVY is expected and was the reason to deprecate the encodings before they are removed.

If we do not deprecate them before removal, how can I proceed with this? If the CI in this repo is not sufficient, how do we know if a PR can be merged?

@clalancette
Copy link
Contributor

Of course, this CI does not test other repos and as far as I understood, causing warnings such as warning: ‘sensor_msgs::image_encodings::YUV422’ is deprecated: use sensor_msgs::image_encodings::UYVY is expected and was the reason to deprecate the encodings before they are removed.

We do not allow warnings in the core. So if something is deprecated here, we need to update the rest of the core that depends on it. Additionally, the #ifdef guards here did not work on Windows, and caused additional regressions there.

If we do not deprecate them before removal, how can I proceed with this?

We should deprecate them before removal.

If the CI in this repo is not sufficient, how do we know if a PR can be merged?

We have CI jobs (which you can see in #256 (comment)) which do test PRs against the entire core. Those need to be run before we merge anything in (we do that for all PRs).

@christianrauch
Copy link
Contributor Author

We have CI jobs (which you can see in #256 (comment)) which do test PRs against the entire core. Those need to be run before we merge anything in (we do that for all PRs).

Are those triggered automatically for PRs to this repo? If so, this did not happen last time. If not, how do I trigger this for a new PR?

@clalancette
Copy link
Contributor

Are those triggered automatically for PRs to this repo?

No, they need to be manually run by a maintainer.

@christianrauch
Copy link
Contributor Author

Ok. I guess I will then run those locally for now and will ask again once I submit a PR again.

For this deprecation change to be accepted again, is it sufficient if all of https://github.com/ros2/ros2/blob/rolling/ros2.repos builds on Linux and Windows without warnings?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants